-
Notifications
You must be signed in to change notification settings - Fork 47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat: add transformation dropdown to histogram #1666
Feat: add transformation dropdown to histogram #1666
Conversation
2a1fdde
to
f688fa4
Compare
...and adjust histogram endpoint call to include histogram concept id in variables list instead of being part of the url, according to the backend changes implemented for this same endpoint.
f688fa4
to
c3d620f
Compare
Please find the detailed integration test report here Please find the ci env pod logs here |
Please find the detailed integration test report here Please find the ci env pod logs here |
...instead, we want the filter just to be recorded as part of the concept item and be reflected in the histogram UI as a line at this point.
Please find the detailed integration test report here Please find the ci env pod logs here |
300af5b
to
5a5bc34
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! I confirmed that I could render the min / max lines and receive different data visualizations with different transformation.
I did a pre-review per your request. I found an ESLINT error and made some code recommendations / nitpicks (feel free to ignore if you don't agree). We might want to break up any new UX requirements into separate tickets if needed. Fixing the accessibility issues found by Axe might not be practical and will need to addressed in the Gen3.2 port.
UX Suggestions:
- Add chart icon to initial screen per design
- Style chart title to be bold
- Format labels for dropdowns to match design
- Implementation missing text version, and text version / Histogram button controls
- Add label for transformation dropdown and place below Cancel / Submit buttons
- Use controlled react components to store state of user selections - I'm seeing a bug where the user inputs disappear after inputting them and changing the transformation type. However, I think the lines associated with the min and max values remain after the chart redrawing.
Notes on Input UX & Error Messaging:
- Currently users can enter non-numeric inputs into the input fields (which then are deleted when leaving focus). Consider adding an error message to let user know only numeric inputs are allowed after user enters an invalid input type.
- Consider adding error messaging for numeric inputs that make the chart render oddly or prevent invalid inputs. For example, if the user adds mins and maximums of -123456789101112131415 and 123456789101112131415 the chart's labels overlap.
- Consider adding error messaging if the min exceeds the max and vice-versa.
- Consider adding error messaging if min exactly equals max
Accessibility Issues:
- In Transformation dropdown: Missing labels for form elements
- In Transformation dropdown: Ensure elements with ARIA roles have all required ARIA attributes

src/Analysis/GWASApp/Components/Diagrams/PhenotypeHistogram/PhenotypeHistogram.jsx
Outdated
Show resolved
Hide resolved
src/Analysis/GWASApp/Components/Diagrams/PhenotypeHistogram/PhenotypeHistogram.jsx
Outdated
Show resolved
Hide resolved
5a5bc34
to
a613c16
Compare
Please find the detailed integration test report here Please find the ci env pod logs here |
Please find the ci env pod logs here |
Please find the detailed integration test report here Please find the ci env pod logs here |
…mes to components and storybooks to better match design
Please find the detailed integration test report here Please find the ci env pod logs here |
b630445
into
feat/filters_and_transformation_main
Link to JIRA ticket if there is one: https://ctds-planx.atlassian.net/browse/VADC-1635
New Features